-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FE] 이벤트 페이지 프로필 표시(로그인 유지) 및 이벤트 페이지 로딩 화면 설정 #853
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생많았습니다 ~~! 크게 리뷰할 부분은 없던 것 같아요,
pr을 읽어보았는데 쿠키말대로 서스펜스를 사용하는게 더 좋겠네요. 움짤로 비교해준 덕에 무엇이 더 좋을지 바로 알 수 있었습니다!
그리고 로딩 화면도 흰 화면만 보이는 것보다 헤더 먼저 보여지는게 더 좋네요. 노션도 그런식으로 로딩이 되고 있더라구요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
잘 작성해 준 것 같아요! 프로필 사진 부분이 좀 작지 않나 하는 감이 있긴 하지만, 다음주 예정된 task인 디자인 검수 단계에서 진행해 보도록 할게요!
useSuspenseQuery
사용 논의에 대한 것은 쿠키가 작성한 부분들에 대해서 동의하지만, 결국 Suspense
와 Errorboundary
를 이용한 데이터 상태에 대한 선언적 UI가 "사용되는 곳"에 사용하는게 중요할 것 같아요.
별도의 요청에 대한 로딩 UI를 보여주지 않는 요청들에 대해서는 useSuspenseQuery
사용을 고려해 봐야 할 것 같습니다!
별도의 로딩 UI를 사용하지 않는데, useSuspenseQuery
와 <Suspense>
를 사용한다면 오히려 코드 이해해 불편하고, 생각하는 대로 작동하지 않는 문제가 있을 것 같아요. 당연히 코드 줄도 길어지게 될 것이구요~!
사소한 리뷰들이기 때문에 approve 하겠습니다~!
/** @jsxImportSource @emotion/react */ | ||
import type {Meta, StoryObj} from '@storybook/react'; | ||
|
||
import getImageUrl from '@utils/getImageUrl'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하지 않는 코드가 있어욘~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
으엇 확인하고 지워놓을게여
width: `${size}px`, | ||
height: `${size}px`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우리의 초반 convention은 rem 단위를 사용하기로 했었는데, rem 단위에 대한 고려가 들어가긴 힘들까 질문드려요~! 아니면, 고정 숫자를 받는 것이 아닌, button과 input에서 했던 것 처럼 semantic하게 설정해줘도 좋겠다 싶습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이 부분 px이 들어가는 것이 괜찮을지 고민을 했었는데 값 전달보다는 시멘틱하게 단위를 나눠서 전달해주는 것이 더 좋은 방향인 것 같아요
@@ -8,6 +8,7 @@ const QUERY_KEYS = { | |||
images: 'images', | |||
kakaoClientId: 'kakao-client-id', | |||
kakaoLogin: 'kakao-login', | |||
userInfo: 'userinfo', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매우매우 사소하지만, pascalCase로 작성하는게 좋지 않을까 싶어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 쿼리키들 말씀하시는거죠? 바꾼다고 해서 큰 상관은 없는 내용이라 바꿔도 좋을 것 같아요. 리팩토링할 때 바꿔봅시다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 뭐든 상관없는데 통일만 되면 될 것 같아요 ㅋㅋㅋㅋㅋ
import {User} from 'types/serviceType'; | ||
|
||
import {BASE_URL} from '@apis/baseUrl'; | ||
import {requestGet} from '@apis/fetcher'; | ||
|
||
export const requestGetUserInfo = async () => { | ||
return await requestGet<User>({ | ||
baseUrl: BASE_URL.HD, | ||
endpoint: `/api/users/mine`, | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백은 아니지만, 이번에 merge할 때 저랑 겹치는 부분이라 유의해서 merge해봅시댱!
export const Profile = ({size = 28, ...profileProps}: ProfileProps) => { | ||
const {theme} = useTheme(); | ||
|
||
return <Image aria-label="프로필 이미지" {...profileProps} css={profileContainerStyle(theme, size)} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-label까지 챙기는 것 👍
@@ -29,13 +29,11 @@ const MobileShareEventButton = ({copyShare, kakaoShare}: MobileShareEventButtonP | |||
}; | |||
|
|||
return ( | |||
<div style={{marginRight: '1rem'}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
div 태그를 제거하게 된 이유가 있을까요?
이 부분 제가 추가했었는데, div 태그로 감싸고 1rem을 marginRight로 줬던 이유가, header 디자인이 통일되지 않아서였던 걸로 기억해요!
혹시 그 부분을 수정해주셔서 코드를 삭제하신건지 궁금해서 남겨욧!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
대신에 이 상위 컴포넌트에서 마진을 주었어요! 상위에서 공유 버튼 이외에 추가로 프로필 컴포넌트가 생겨서 두 개를 감싸는 Flex 컴포넌트에 마진을 주었습니다
<Flex alignItems="center" gap="0.75rem" margin="0 1rem 0 0">
{isMobile ? (
<MobileShareEventButton copyShare={trackLinkShare} kakaoShare={trackKakaoShare} />
) : (
<DesktopShareEventButton onCopy={trackLinkShare} />
)}
{isKakaoUser && (
<Link to={ROUTER_URLS.myPage}>
<Profile src={event.userInfo.profileImage ?? getImageUrl('runningDog', 'png')} size="medium" />
</Link>
)}
</Flex>
@@ -27,12 +27,10 @@ const DesktopShareEventButton = ({onCopy}: DesktopShareEventButtonProps) => { | |||
}; | |||
|
|||
return ( | |||
<div style={{marginRight: '1rem'}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 div 삭제도 마찬가지입니댜!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 위와 마찬가지 이유입니다~
</TopNav.Item> | ||
<TopNav.Item displayName="홈" routePath="/home" /> | ||
<TopNav.Item displayName="관리" routePath="/admin" /> | ||
</TopNav> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로딩중일때, 로딩중이라는 이미지나 문구가 있으면 더 좋을 것 같긴해요!
그냥 TopNav만 띄워지면 어떤 상황인지 유저가 인지하기 어려울 것 같아서용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그 부분이 살짝 우려가 되긴 했어요.. 어떤 내용이 들어오면 좋을지 이건 이따가 회의 때 이야기해보면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이미지같이 덩치가 크면 로딩 페이지 -> 가려는 페이지 사이에서 깜빡이는게 크게 체감될 것 같단 생각이 들어용.. 그래서 스켈레톤이나 작은 로딩 아이콘을 두는게 어떨까 싶습니다!
<DesktopShareEventButton onCopy={trackLinkShare} /> | ||
)} | ||
{isKakaoUser && ( | ||
<Link to={ROUTER_URLS.myPage}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useNavigate가 아닌 Link 사용하신 이유가 있을까요?? 단순히 궁금해서 여쭤봐요! 웹 접근성 때문에 Link를 사용하신 건지..🧐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프로필을 클릭했을 때 단순히 경로만 바뀌는 역할만해서 Link를 사용했던 것 같아요! 접근성은 생각하지 못 했는데 접근성의 이유가 될 수도 있을 것 같아요. button이 아닌데 onClick을 주는 것도 어색하다고 생각할 수 있을 것 같기도 해요
쿠키~! 구현해주신 내용 잘 확인했습니다! 꼼꼼하게 PR 남겨줘서 이해하기 쉬웠어요. |
issue
구현 사항
Profile 디자인 컴포넌트 생성
Image 컴포넌트를 그대로 이용해서 구현해도 됐었지만, 프로필 이미지라는 것은 서비스의 성격을 띈다고 생각했습니다. 그래서 프로필의 스타일을 가진 (둥글고 사진이 들어가는) 디자인 컴포넌트를 제작하여 이용하도록 구성했습니다.
구현은 이미지 컴포넌트에 aria-label로 프로필 이미지를 의미하도록 설정해줬으며, 스타일을 적용해줬습니다.
size의 기본값은 28px으로 외부에서 이를 변경할 수 있도록 확장했습니다. 나머지 props는 ImageProps와 동일합니다.
프로필 버튼을 클릭하면 마이페이지로 이동하도록 구현
이벤트 페이지에서 카카오 로그인 유저일 때 프로필이 보이도록 설정했으며, 이를 클릭할 때 제가 저번 pr에 퍼블리싱한 마이페이지가 보이도록 했습니다
카카오 유저인지 확인하여 프로필 버튼을 보여주는 기능
비회원/회원 여부, 닉네임, 카카오 이미지 정보를 조회할 수 있는 api(/api/users/mine)가 추가되었습니다.
Get 메서드이며 User 타입(nickname, bankName, accountNumber)에 isGuest, profileImage가 추가된 응답을 받습니다.
profileImage 정보를 이용하여 프로필 이미지를 보여주며, 만일 이 값이 null일 경우 행댕이 이미지가 보이도록 설정했습니다.
이 api는 이벤트에 한정되는 도메인이 아니라 서비스 전체의 성격을 띈다고 생각했습니다. 실제로 백엔드에서도 이벤트와 유저는 다른 성격으로 인지해서 이를 분리해서 관리하고 있습니다.
현재 EventLoader를 사용해서 Reports, Steps, AllMembers 데이터를 미리 불러와 놓고 있습니다. 여기에 추가하게 되면 간단하지만 이벤트로더는 이벤트와 관련된 api를 불러와야 한다고 판단하여 EventLoader가 아닌 내부에서 호출하기로 결정했습니다.
제가 생각하는 useSuspenseQuery를 사용해야 하는 경우를 두 가지로 정리해보겠습니다.
이 경우에는 사용자 경험을 위해서 데이터를 보장 받아야 할 필요가 있다고 판단했습니다.
조건부 렌더링을 사용하여 프로필 컴포넌트를 보여주고 있는데 여기서 값을 보장할 수 없다면 깜빡거리는 현상을 일으키기 때문입니다. 아래 useQuery를 적용할 경우, useSuspenseQuery를 적용할 경우에 대한 gif를 아래 비교 첨부하겠습니다.
이벤트 페이지 로딩 화면 설정
이벤트 로딩 화면을 보여주도록 설정했습니다. 이전에는 데이터를 불러올 때까지 흰 화면만 보여주게 되어 지금 데이터를 불러오고 있는 것인지, 아니면 경로가 잘못되어 이상한 화면을 보여주고 있는 것인지 인지하기 어려웠기 때문입니다.
그래서 이벤트 페이지의 일부라도 보여주어 지금 데이터를 불러오고 있는 중이라는 것을 알려주도록 변경했습니다.
여기서 EventLoader의 useQueries를 useSuspenseQueries로 변경했습니다. 그 이유는 위에서 설명한 2) 선언적 로딩 화면을 보여주고 싶을 때를 만족하기 때문입니다. 데이터를 불러오는 동안 대체로 보여줄 Fallback이 필요하고 이를 선언적으로 처리하기 위해 useSuspenseQueries로 변경했습니다.
그리고 라우터에서 Suspense 컴포넌트로 감싸서 Fallback을 보여주도록 했습니다.
이벤트 페이지 헤더에는 흔듯콘, 홈, 관리를 포함하는 TopNav와 초대하기, 프로필을 감싸는 두 구역으로 나뉘어있습니다.
Fallback이 보여진다는 것은 아직 이벤트 정보를 불러오고 있다는 중을 의미합니다. 그래서 이 때 초대하기나 프로필을 누르는 액션이 일어난다면 버그가 발생할 수 있다 생각했습니다. 그래서 헤더는 아래 사진대로 TopNav만 보이도록 했습니다.
중점적으로 리뷰받고 싶은 부분
이전에 로딩 중에 흰 화면이 보이는 것을 이제야 보완했어요.. 지금 방식 보다 더 나은 방식이 있다면? 아니면 더 나은 로딩 화면 디자인이 있다면 의견 주세요!
논의하고 싶은 부분
제가 생각하는 useSuspenseQuery의 사용 조건에 대해 공감하시는지 궁금합니다!
다른 의견이 있다면 코멘트로 남겨주세요
🫡 참고사항